Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

tests/ui-tools.spec.ts (2)

790-805: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicates the new scrollPrimarySurface helper instead of reusing it.

This test manually re-implements scrolling #main-content (setting scrollTop and dispatching a bubbling scroll event) rather than calling the shared scrollPrimarySurface helper introduced in tests/playwright-scroll.ts. Beyond duplicating logic, the manual version dispatches with bubbles: true while the helper does not — an inconsistency that can mask real differences in how the app's scroll-hide reporter is wired.

♻️ Suggested refactor

-    const scroller = page.locator("`#main-content`");
-    // Step scroll down so the shell's scroll reporter sees deliberate movement.
-    for (const offset of [40, 80, 120, 160, 200]) {
-      await scroller.evaluate((node, top) => {
-        node.scrollTop = top;
-        node.dispatchEvent(new Event("scroll", { bubbles: true }));
-      }, offset);
-    }
+    // Step scroll down so the shell's scroll reporter sees deliberate movement.
+    for (const offset of [40, 80, 120, 160, 200]) {
+      await scrollPrimarySurface(page, offset);
+    }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/ui-tools.spec.ts` around lines 790 - 805, The scroll setup in the
ui-tools spec duplicates the shared scrolling logic instead of using the new
scrollPrimarySurface helper. Replace the manual `#main-content` scrollTop updates
and bubbling scroll dispatch in this test with calls to scrollPrimarySurface
from tests/playwright-scroll.ts, and keep the test aligned with the helper’s
event behavior so the scroll-hide reporter is exercised consistently.

---

433-444: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Redundant fallback after null assertion.

mainBox?.height ?? 820 is dead code for the happy path since expect(mainBox).not.toBeNull() on the previous line already guarantees mainBox is non-null when the test proceeds; the 820 magic number is essentially unreachable and just obscures intent.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/ui-tools.spec.ts` around lines 433 - 444, The mainBox null fallback in
the ui-tools test is redundant because the preceding assertion already
guarantees a value, so remove the dead fallback and make the intent explicit.
Update the search position checks in the same test block to use the non-null
mainBox directly after the expect(mainBox).not.toBeNull() assertion, and keep
the comparison anchored on the existing mainBox, searchBox, and headingBox
locators.

src/components/DocumentViewer.tsx (1)

1941-1958: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

DOM-ID coupling + whole-body MutationObserver for scroll-container resolution.

shellScrollContainer is resolved via getElementById("main-content") inside a MutationObserver watching document.body with { childList: true, subtree: true } for the component's entire lifetime. This works, but:

• It couples DocumentViewer to a magic string ID owned by global-mockup-search-shell.tsx, with no compile-time contract — a rename there silently breaks scroll-hide here.
• The observer callback fires on any DOM mutation anywhere in the app (sheets opening, image lists re-rendering, etc.), not just changes relevant to #main-content.

Since the shell already owns a mainRef/scroll-root value, consider exposing it via a small context (or prop) instead of DOM lookups, so this becomes a typed dependency rather than an implicit string-ID contract.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/DocumentViewer.tsx` around lines 1941 - 1958, The scroll
container lookup in DocumentViewer is tightly coupled to the hardcoded
"main-content" DOM id and uses a broad MutationObserver over document.body for
the component lifetime. Replace this implicit DOM query in the
shellScrollContainer/useHideOnScroll setup with a typed dependency from the
shell, such as a small context or prop backed by the shell’s mainRef/scroll-root
value, so DocumentViewer no longer depends on a magic string or whole-app
mutation watching.

src/components/clinical-dashboard/favourites-home-page.tsx (1)

20-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider reusing ModeHomeMain instead of duplicating its wrapper styling.

This <main> re-implements most of ModeHomeMain's classes (flex min-h-0 w-full flex-1 flex-col bg-[color:var(--background)] ... text-[color:var(--text)]) but diverges in a few subtle ways that look unintentional:

• Uses py-4/sm:py-5 instead of the separate pt-[clamp(...)]/pb-4/sm:pt-[clamp(...)]/sm:pb-[clamp(...)] split that mode-home-template.tsx now uses (line 130).
• Uses sm:px-4 here vs sm:px-6 in ModeHomeMain (line 130).
• Omits items-center justify-center (intentional here, since Favourites stays top-aligned per the comment in ClinicalDashboard.tsx around line 2914-2917), but that intent isn't documented in this file.

Since both wrappers now implement the same "edge-to-edge main + inner px-4 sm:px-0 div" pattern for the same underlying reason (composer-reserve edge-to-edge layout), extending ModeHomeMain with an alignment option (e.g., align="start" | "center") and reusing it here would keep the two paths from drifting further apart as the mobile shell layout continues to evolve.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/clinical-dashboard/favourites-home-page.tsx` around lines 20 -
32, The Favourites home page is duplicating `ModeHomeMain` wrapper styling and
has already drifted in padding and alignment details. Update
`FavouritesHomePage` to reuse `ModeHomeMain` (or extend `ModeHomeMain` with an
alignment option such as start vs center) instead of hardcoding the `<main>`
classes here, and keep the inner `px-4 sm:px-0` layout. Make sure the special
top-aligned behavior for favourites is preserved explicitly through
`ModeHomeMain`, so this file only passes the needed props and `FavouritesHub`
stays unchanged.

✅ Autofix completed